Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed backup suite to be more generic & changed mssql app to mysql #565

Closed
wants to merge 1 commit into from
Closed

Changed backup suite to be more generic & changed mssql app to mysql #565

wants to merge 1 commit into from

Conversation

mperetzred
Copy link
Contributor

  • Changed backup suite to be more generic so it could be used later with other CSI drivers, different backup spec, etc.
  • Changed mssql app to mysql because mssql fails with restic as it uses deploymentconfig (restore gets stuck on InProgress; known bug).

@mperetzred mperetzred marked this pull request as draft February 6, 2022 18:16
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2022
@mperetzred mperetzred marked this pull request as ready for review February 9, 2022 14:17
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2022
@openshift-ci openshift-ci bot requested a review from kaovilai February 9, 2022 14:18
@mperetzred
Copy link
Contributor Author

/retest

@mperetzred
Copy link
Contributor Author

/test

@mperetzred
Copy link
Contributor Author

/test 4.8-operator-e2e

@mperetzred
Copy link
Contributor Author

/test 4.7-operator-e2e

@openshift-ci
Copy link

openshift-ci bot commented Feb 9, 2022

@mperetzred: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

  • /test 4.7-ci-index
  • /test 4.7-images
  • /test 4.7-operator-e2e
  • /test 4.7-operator-unit-test
  • /test 4.8-ci-index
  • /test 4.8-images
  • /test 4.8-operator-e2e
  • /test 4.8-operator-unit-test

Use /test all to run all jobs.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mperetzred
Copy link
Contributor Author

/test 4.7-operator-e2e

@mperetzred
Copy link
Contributor Author

/test 4.8-operator-e2e

Mathieu-Ferraton
Mathieu-Ferraton reviewed Feb 10, 2022
View reviewed changes
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-template.yaml Outdated
port: 3306
selector:
name: mysql
- apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PVC was already defined above. It should be removed from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

tests/e2e/sample-applications/mysql-persistent/mysql-persistent-template.yaml Outdated
spec:
accessModes:
- ReadWriteOnce
storageClassName: gp2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gp2 storageClass is specific to AWS, and the deployment fails due to pending pvc for clusters deployed outside aws. Maybe this could be left blank, so that we use the clsuter's default storage class?

Copy link
Contributor Author

@mperetzred mperetzred Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agree. The MSSQL also had the storageclasses hardcoded, so I made it in a similar way, cause I didn't want to change the logic. I plan later to refactor the apps, but not as part of this PR cause it's already too big.

Mathieu-Ferraton
Mathieu-Ferraton reviewed Feb 10, 2022
View reviewed changes
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi-template.yaml Outdated
selector:
name: mysql
- apiVersion: v1
kind: PersistentVolumeClaim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, the pvc is declared twice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@mperetzred mperetzred force-pushed the backup_change branch 3 times, most recently from 9317125 to 9d8fcee Compare February 10, 2022 15:30
@weshayutin
Copy link
Contributor

@dymurray @kaovilai if we change the e2e mssql to mysql should we also change the example app to mysql in the same review or a follow up review? https://github.com/openshift/oadp-operator/blob/master/docs/examples/stateful.md

@kaovilai
Copy link
Member

Could do in a follow up.. or not touch it at all. The problem we've been seeing with mssql in e2e environment doesn't appear to be resolved by the switch to mysql as we are still reliant on provisioning pvcs.
https://coreos.slack.com/archives/C0144ECKUJ0/p1644507358477549?thread_ts=1644332499.312449&cid=C0144ECKUJ0

@weshayutin
Copy link
Contributor

@tiger my vote would be to change the example app as the app does not consistently come up properly most times.. probably works 50% of the time at best outside of e2e. Let's get it down in a follow up patch would be my vote. @mperetzred

@weshayutin
Copy link
Contributor

Unrelated to this change... please hold off on merging so we can land multicloud tests.
#568
Sorry for the inconvience folks. @dymurray

@kaovilai
Copy link
Member

/hold for #568

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 10, 2022
@mperetzred mperetzred force-pushed the backup_change branch 4 times, most recently from 9b721a9 to 942b523 Compare February 14, 2022 18:29
@mperetzred mperetzred force-pushed the backup_change branch 3 times, most recently from 5d2a5ff to 6054ad9 Compare February 15, 2022 14:13
@weshayutin
Copy link
Contributor

@mperetzred just in case you didn't see.. #565 merged. So you are unblocked :) Thank you for your patience.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2022
@kaovilai
Copy link
Member

@mperetzred just in case you didn't see.. #565 merged. So you are unblocked :) Thank you for your patience.

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2022
@mperetzred mperetzred force-pushed the backup_change branch 12 times, most recently from a59b9ce to d6265b6 Compare March 2, 2022 16:38
@openshift-ci
Copy link

openshift-ci bot commented Mar 2, 2022

@mperetzred: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.8-operator-e2e a384532 link true /test 4.8-operator-e2e
ci/prow/4.7-operator-e2e a384532 link true /test 4.7-operator-e2e
ci/prow/4.9-operator-e2e a384532 link true /test 4.9-operator-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 6, 2022

@mperetzred: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mperetzred
Copy link
Contributor Author

closing up this PR, will break it into few PRs as there is too much code changes here

@mperetzred mperetzred closed this Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mathieu-Ferraton Mathieu-Ferraton Mathieu-Ferraton left review comments

@kaovilai kaovilai kaovilai approved these changes

@pranavgaikwad pranavgaikwad Awaiting requested review from pranavgaikwad

@sseago sseago Awaiting requested review from sseago

Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mperetzred @weshayutin @kaovilai @Mathieu-Ferraton